fix(imap): handle korean Windows encoding ks_c_5601-1987/ks_c_5601-1989 - #12654
fix(imap): handle korean Windows encoding ks_c_5601-1987/ks_c_5601-1989#12654ChristophWurst wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Nextcloud Mail issue #11416 by improving IMAP charset handling for Outlook messages that label Korean content using non-mbstring charset names (ks_c_5601-1987 / ks_c_5601-1989), preventing conversion failures during message processing.
Changes:
- Add a charset normalization step mapping
ks_c_5601-1987/ks_c_5601-1989toUHC(CP949/Windows-949). - Apply the normalized charset during
mb_convert_encoding()/iconv()conversion in the IMAP charset converter. - Add unit test coverage for both Outlook Korean charset labels.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/IMAP/Charset/Converter.php |
Adds charset normalization and uses it during conversion to avoid failures on Outlook Korean charset aliases. |
tests/Unit/IMAP/Charset/ConverterTest.php |
Adds unit tests validating conversion of ks_c_5601-1987 and ks_c_5601-1989 content to UTF-8. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for taking over; looks good! When I looked at it a while back, I'd bumped into some blockers and didn't find the time to continue.
The list also contains "BASE64", "UUENCODE", "HTML-ENTITIES", and "Quoted-Printable". Request the aliases for those; does trigger a deprecation warning 🙈 What I had in mind was similar to what also Josh started with #12221. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/backport to stable5.11 |
b758b49 to
a588f03
Compare
| * Map of unsupported charset names to their mbstring equivalents. | ||
| * Keys must be lowercase for case-insensitive lookup. | ||
| * | ||
| * @see http://lists.w3.org/Archives/Public/ietf-charsets/2001AprJun/0030.html | ||
| */ | ||
| private const CHARSET_MAP = [ | ||
| 'ks_c_5601-1987' => 'UHC', | ||
| 'ks_c_5601-1989' => 'UHC', | ||
| ]; | ||
|
|
||
| /** | ||
| * Normalize charset names for mbstring compatibility. | ||
| * | ||
| * Maps unsupported charset names to their mbstring equivalents. | ||
| * Notably, handles Korean encodings used by Outlook: | ||
| * - ks_c_5601-1987 and ks_c_5601-1989 are mapped to UHC (Windows-949/CP949) |
| if (in_array($normalizedCharset, mb_list_encodings(), true)) { | ||
| $converted = mb_convert_encoding($data, 'UTF-8', $normalizedCharset); | ||
| } else { | ||
| $converted = @iconv($normalizedCharset, 'UTF-8', $data); |
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe charset converter now normalizes Korean aliases, caches supported encodings, handles ChangesCharset conversion handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The PR improves Korean Windows charset handling in IMAP messages, but some CP949 messages may still be decoded as Latin-1 or processed through an inconsistent conversion path, causing Korean text to appear garbled. The change is otherwise localized and mergeable with explicit owner awareness or follow-up on these bounded correctness risks. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes address issue Full details: Out of Scope Changes checkExplanation The production and test changes remain within scope for issue ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Team
Run ID: d31edf28-485a-495c-839b-c218eb3c545f
📒 Files selected for processing (2)
lib/IMAP/Charset/Converter.phptests/Unit/IMAP/Charset/ConverterTest.php
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| /** @var list<string>|null $encodings */ | ||
| static $encodings = null; | ||
| if ($encodings === null) { | ||
| $encodings = mb_list_encodings(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- applicable repository guidance ---'
find /tmp/coderabbit-repo-knowledge/nextcloud-mail-16433369 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- changed hunk ---'
git diff -- lib/IMAP/Charset/Converter.php
printf '%s\n' '--- file outline and relevant source ---'
ast-grep outline lib/IMAP/Charset/Converter.php
sed -n '1,180p' lib/IMAP/Charset/Converter.php
printf '%s\n' '--- direct references ---'
rg -n 'mbEncodings|mb_encoding_aliases|mb_list_encodings|iconv|convert' lib tests 2>/dev/null | head -160Repository: nextcloud/mail
Length of output: 15971
🌐 Web query:
PHP mb_list_encodings mb_encoding_aliases CP949 UHC alias behavior exact documentation
💡 Result:
In PHP, UHC (Unified Hangul Code) is the formally recognized encoding name used by the mbstring extension [1][2]. It serves as the primary identifier, and CP949 is explicitly documented as an alias of UHC [1][2]. Behavior and Technical Details: 1. mb_list_encodings: When you call mb_list_encodings, the output will include UHC in the list of supported encodings, but it generally will not list CP949 as a separate, distinct entry [3][4]. 2. mb_encoding_aliases: If you pass 'UHC' to the mb_encoding_aliases('UHC') function, it will return an array that includes 'CP949' among its aliases [5][6]. 3. Normalization: Because mbstring treats them as the same underlying character set, you can use 'CP949' as an input parameter in functions like mb_convert_encoding or mb_detect_encoding because the library internally maps it to UHC [1][2]. In summary, UHC is the "canonical" name within PHP's mbstring implementation, while CP949 is treated as an alias [1][2]. They are functionally interchangeable in PHP functions that accept encoding names [1][2].
Citations:
- 1: https://www.php.net/manual/en/mbstring.supported-encodings.php
- 2: https://www.php.net/mbstring.supported-encodings
- 3: https://www.php.net/manual/en/function.mb-list-encodings.php
- 4: https://www.php.net/mb_list_encodings
- 5: https://www.php.net/manual/en/function.mb-encoding-aliases.php
- 6: https://www.php.net/mb_encoding_aliases
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- PHP version and mbstring requirements ---'
rg -n '"php"|mbstring|platform' composer.json composer.lock lib tests 2>/dev/null | head -100
printf '%s\n' '--- charset converter tests ---'
sed -n '1,175p' tests/Unit/IMAP/Charset/ConverterTest.php
printf '%s\n' '--- repository-wide review and test conventions ---'
cat /tmp/coderabbit-repo-knowledge/nextcloud-mail-16433369/conventions/repo-wide.md
cat /tmp/coderabbit-repo-knowledge/nextcloud-mail-16433369/conventions/tests-unit.mdRepository: nextcloud/mail
Length of output: 14730
🌐 Web query:
PHP iconv encoding names platform dependent CP949 Windows-949 libiconv glibc documentation
💡 Result:
The availability and naming of character encodings in PHP's iconv extension are heavily dependent on the underlying system's iconv implementation [1][2]. PHP acts as an interface to the iconv library provided by the host operating system, which is typically glibc on Linux/Unix systems or GNU libiconv on Windows [1][3][4]. Because there is no single standardized naming convention for character encodings across these different libraries, encoding names such as CP949 and Windows-949 can behave inconsistently depending on the platform [1][5]. Key points regarding these encodings and platform dependencies: 1. Platform Dependence: PHP's iconv extension does not define its own encoding list. Instead, it relies on the library present on the system (e.g., glibc or libiconv) [1][2]. These libraries often have different sets of supported names, aliases, and internal mappings for specific character sets [6][5]. 2. CP949 vs. Windows-949: CP949 is widely recognized as the Microsoft Windows code page for Korean (Unified Hangul Code) [7][8][9]. While GNU libiconv explicitly supports CP949 [10][11][12], glibc's support and its recognition of various aliases may differ [6][5]. Users often encounter discrepancies where an encoding name accepted by libiconv on Windows causes an error or behaves differently when used with the glibc-based iconv on Linux [1][4]. 3. Recommended Practice: Because PHP's iconv implementation is system-dependent, applications requiring cross-platform consistency are generally advised to use the mbstring extension where possible, as it provides more consistent encoding support across different environments [13]. If iconv must be used, developers should be aware that the specific list of supported encoding names is determined by the underlying library and can be verified via the phpinfo function, which reports the active ICONV_IMPL [3]. For troubleshooting specific encoding issues, checking the installed iconv implementation and version in your environment is the recommended first step [3][2].
Citations:
- 1: https://www.php.net/manual/en/function.iconv.php
- 2: https://www.php.net/iconv
- 3: https://github.com/php/php-src/blob/fe52e5b6/ext/iconv/iconv.c
- 4: GitHub pull request 9149 in php/php-src (link omitted to avoid creating a cross-reference)
- 5: https://www.sourceware.org/glibc/manual/2.44/html_node/Generic-Conversion-Interface.html
- 6: https://sourceware.org/glibc/manual/2.41/html_node/glibc-iconv-Implementation.html
- 7: https://github.com/php/php-src/blob/fe52e5b6/win32/cp_enc_map_gen.c
- 8: https://learn.microsoft.com/en-us/windows/win32/Intl/code-page-identifiers
- 9: https://docs.oracle.com/cd/F16635_01/html/E73433/iconv-ko-7.html
- 10: https://fossies.org/dox/libiconv-1.19/localcharset_8h_source.html
- 11: https://fossies.org/dox/libiconv-1.19/cp949_8h_source.html
- 12: http://svn.code.sf.net/p/pure-data/svn/vendor/libiconv/1.13/lib/cp949.h
- 13: https://www.php.net/manual/en/mbstring.supported-encodings.php
Normalize mbstring aliases before selecting the conversion backend.
Converter::convert() compares the declared charset with mb_list_encodings(). CP949 is an accepted mbstring alias for UHC, but it is not listed as CP949, so this path sends it to platform-dependent iconv(). If iconv() rejects the alias, the fixed Latin fallback can corrupt Korean bytes. Include mb_encoding_aliases() in the cache or canonicalize the charset before comparison.
af9564b to
a8c2155
Compare
Assisted-by: OpenCode:claude-haiku-4-5 Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com>
a8c2155 to
5131dac
Compare
Fixes #11416
Trick borrowed from https://github.com/bytestream/Util/blob/45a9c3c14e70eec9df5d117a2d4941b097872672/lib/Horde/String.php#L847-L867.
Tested with the email from the ticket - threw ValueException before, is converted now ✔️
AI-assisted: OpenCode (Claude Haiku 4.5)
Summary by CodeRabbit
ks_c_5601-1987andks_c_5601-1989.